-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace vendored SIMD Everywhere by prefix/system install #10961
base: master
Are you sure you want to change the base?
Replace vendored SIMD Everywhere by prefix/system install #10961
Conversation
efd9e74
to
3985104
Compare
3985104
to
2ac6089
Compare
@@ -426,6 +426,14 @@ function(target_export target) | |||
DESTINATION "${package_destination}" | |||
COMPONENT Development | |||
${exclude_variant}) | |||
|
|||
if(target STREQUAL libobs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we also need to export SIMDE here? Are SIMDE types used in function prototypes or inline functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The headers include a SIMDE header which make it a dependency of libobs header and so the CMake package needs the finder to be installed alongside the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the question was more about which ones? Because SIMDE is an implementation detail of libobs
internals and should not be exposed publicly. It doesn't make sense to expose it from an API point of view and if we do that seems like a mistake to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util/sse-intrin.h
(which hard depends on SIMDE) used in some installed headers:
grep -r "sse-intrin.h" /usr/include/obs/.
/usr/include/obs/./graphics/quat.h:#include "../util/sse-intrin.h"
/usr/include/obs/./graphics/vec3.h:#include "../util/sse-intrin.h"
/usr/include/obs/./graphics/vec4.h:#include "../util/sse-intrin.h"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right and of course we inlined all those functions so we created a tight coupling between API users and the internal data types..
That should have never happened, but I guess that's water under the bridge and can be cleaned up once the current API is not used externally anymore in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this thread can be resolved as "todo in a future effort".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally it should be fixed before making it an external dependency, because by fixing that, we exposed another open wound (namely that it now becomes an explicit public 3rd party dependency) and we just trade one problem (vendored dependency) for another (internal dependency becomes a hard public external dependency) even though there is no architectural reason for that to be the case.
We're doctoring around symptoms again and make a change that is mostly cosmetic instead of fixing the actual issue. But it's a fight I've never won, so I gave up fighting it.
56c0582
to
a86a165
Compare
2736151
to
ebbb3a1
Compare
SIMD Everywhere finder needs to be installed alongside libobs CMake package since its headers depends on it. macOS AVX intrinsics is included to make sure that AVX defines are present.
ebbb3a1
to
46ab8e1
Compare
Description
Important
Depends on:
Drop of the CMake legacy (I can implement legacy path if really needed)Close #5067
Replace vendored SIMD Everywhere by prefix/system install.
SIMD Everywhere finder needs to be installed alongside libobs CMake
package since its headers depends on it.
macOS AVX intrinsics is included to make sure that AVX defines are
present.
Motivation and Context
Reduce in-tree vendored deps in OBS Studio repo.
How Has This Been Tested?
Build in CI on my fork.
Types of changes
Checklist: